Skip to content

fix(mode): fail loud on a half-configured client instead of silently reading the on-box store - #60

Merged
andrei-hasna merged 3 commits into
mainfrom
fix/b71ec0eb-loud-half-configured-client
Jul 30, 2026
Merged

fix(mode): fail loud on a half-configured client instead of silently reading the on-box store#60
andrei-hasna merged 3 commits into
mainfrom
fix/b71ec0eb-loud-half-configured-client

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Finding: 0.2.92's local default was DELIBERATE HARDENING, not an accidental regression

Read the diff before choosing a fix. The behaviour change came in with PR #43
fix/f22a263f-explicit-mode-and-egress-guard
(merge 4226c25, 2026-07-28),
which sits squarely inside the published 0.2.91 -> 0.2.92 window:

0.2.91 published 2026-07-24T15:52Z
PR #43 merged 2026-07-28T04:23
0.2.92 published 2026-07-30T13:09Z

Nothing about it is accidental. It added src/knowledge-mode.ts (191 lines,
entirely about this one decision), tests/knowledge-mode.test.ts (215 lines) with
tests named "REGRESSION: url + key with no mode var stays LOCAL", a new
knowledge mode command, and README changes. The module header states the rule
and the incident it came from: an ambient HASNA_KNOWLEDGE_API_URL +
HASNA_KNOWLEDGE_API_KEY exported in a login shell and inherited by every tmux
pane silently routed a test suite's writes at the live store, surfacing as a
flood of unrelated failures. It also closed a second inference layer in
@hasna/contracts via pinnedTransportEnv.

So restoring 0.2.91's presence-based routing would reintroduce a known,
documented data-integrity incident. This PR does not do that.

What this PR changes

It keeps the strictness and removes the silence it left behind.

Measured on this branch before the fix, in a sandbox HOME with the pointer set
and no mode var:

$ knowledge list --json
{ "ok": true, "total": 0, "items": [], "store_exists": false }   # exit 0

A successful-looking empty page on a machine explicitly pointed at a store
holding 869 entries (station01 measured 98 local vs 869 hosted). knowledge mode
reported the ignored pointer correctly the whole time — but nothing runs
knowledge mode before trusting a list.

Both silent readings are wrong for the same reason: the environment is
ambiguous and the client picked an answer instead of saying so.

assertKnowledgeModeSelected is a gate on top of the resolution, not a change
to it. resolveKnowledgeModeSelection stays total and non-throwing, because
knowledge mode resolves through it and must keep answering in exactly the
environment the gate rejects — a guard fused into the resolver would take the
diagnostic down with the defect.

Scope is deliberately narrow so the check does not cry wolf:

  • an API URL provokes it; an API key alone does not — a key with no URL
    points at no store, so there is nothing to be ambiguous about
  • MODE=cloud, MODE=local, and an explicit --store all pass — each is the
    operator saying which store they meant
  • a clean local install (nearly every invocation) is untouched

Applied at the CLI and at the MCP item-store resolver. The MCP needed it most: an
agent calling an item tool never sees a knowledge mode line, so it reads the
empty result as an empty corpus.

Regression test first

bf1e360 (tests) is committed before abfb24c (fix). Verified red against
the test-only commit in a detached worktree, with the fix absent:

$ git worktree add --detach ../red bf1e360
$ bun test tests/cli.test.ts -t "half-configured CLI fails loudly"
    expect(result.exitCode).not.toBe(0)
    error: expect(received).not.toBe(expected)
    Expected: not 0
  (fail) REGRESSION: `list --json` with an API URL and no mode var exits non-zero
  5 pass, 1 fail

The one failing assertion is exactly the defect: exit 0 where it must be
non-zero. The 5 that pass on the old code are the guard rails — knowledge mode
still answering, MODE=local still listing, --store still listing, and a clean
env untouched — so the new suite is not simply "everything errors now".

Reviewer notes

  • Blast radius. On a host with HASNA_KNOWLEDGE_API_URL set and no mode var,
    every store-touching command now errors instead of returning local data. That
    is the intent, and the remedy is one variable, named in the message. Worth a
    deliberate look given the fleet-wide install.
  • Contradiction with the 2026-07-29 owner directive. That directive removes
    deployment modes and says the OSS client picks its store by which connection
    is configured, with "both configured" a hard boot error. HASNA_KNOWLEDGE_STORAGE_MODE
    is therefore on death row, and this PR's error message tells operators to set it.
    I still think this is right to land now — it makes today's shipped contract
    honest rather than silent, and the "ambiguous config is a hard error" concept is
    exactly what survives the mode var's removal; only the remedy text changes. But
    it is a real tension between two owner-level decisions and the reviewer should
    see it rather than discover it. There is already a
    task/85b4eaca-remove-deployment-modes branch in this repo.
  • Full suite not run on this branch — the box is capped at 4 concurrent suites and
    I was heavily throttled. Targeted red evidence above; CI is the green gate.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…ilent empty read

0.2.91 honoured HASNA_KNOWLEDGE_API_URL. 0.2.92 does not: PR #43 made mode
selection explicit-only, so a URL pointer with no HASNA_KNOWLEDGE_STORAGE_MODE
resolves to the on-box store. That was deliberate and correct — a pointer
exported in a login shell must never route writes at the live store.

What it left behind is the mirror-image failure. `knowledge list --json` on a
machine explicitly pointed at the hosted store answers
`{"ok": true, "total": 0, "items": []}` with exit 0. Measured on station01: 98
entries locally against 869 in the store the URL names. Only `knowledge mode`
ever mentioned the ignored pointer, and nothing runs `knowledge mode` before
trusting a list.

These tests fail on current HEAD by design; the fix follows. They assert the
guard fires only on a genuinely ambiguous environment (a URL pointer with no
mode var), that both remedies are named, that no pointer VALUE reaches stderr,
and — the part that is easy to lose — that `knowledge mode` still answers in
exactly the environment the guard rejects.

Agent: vespasian
…the on-box store

Keeps 0.2.92's strictness — a pointer still never selects the backend — and
removes the silence it left behind. An API URL with no mode variable is an
ambiguous environment, and the client was resolving it to local and answering
`{"ok": true, "total": 0, "items": []}` with exit 0.

`assertKnowledgeModeSelected` is a gate on top of the resolution, not a change
to it. `resolveKnowledgeModeSelection` stays total and non-throwing because
`knowledge mode` resolves through it and has to keep answering in exactly the
environment the gate rejects — a guard fused into the resolver would take the
diagnostic down with the defect.

Scope is deliberately narrow so the check does not cry wolf:
  - an API URL provokes it; an API key alone does not, because a key with no
    URL points at no store and there is nothing to be ambiguous about
  - `MODE=cloud`, `MODE=local` and an explicit `--store` all pass — each one is
    the operator saying which store they meant
  - a clean local install, which is nearly every invocation, is untouched

Applied at the CLI and at the MCP item-store resolver. The MCP is the surface
that needed it most: an agent calling an item tool never sees a `knowledge mode`
line, so it reads the empty result as an empty corpus.

Agent: vespasian
…-configured guard

The committed generated artifacts were last rebuilt at d141994 and did not
contain this PR's own new symbols, so the required CI check
`Verify generated artifacts` failed deterministically.

This was not merely a red light. package.json maps bin.knowledge-mcp to
bin/knowledge-mcp.js, so the MCP half of the fix did not exist in the artifact
that actually executes — a half-configured client reached through the MCP would
still have silently fallen back to the on-box store.

Rebuilt with `bun run build` under bun 1.3.14, matching the bun-version pinned
in .github/workflows/ci.yml.

Positive control on the rebuilt bundles:
  bin/knowledge.js       HalfConfiguredKnowledgeClientError x1
                         (assertKnowledgeModeSelected is name-mangled by --minify)
  bin/knowledge-mcp.js   HalfConfiguredKnowledgeClientError x3,
                         assertKnowledgeModeSelected x2

bin/knowledge-serve.js and dist/*.js are byte-identical to what was already
committed, which is correct: only src/cli.ts and src/mcp.js reach the new
symbols. dist/knowledge-mode.d.ts is the tsc typing emit for the new module.

Agent: vespasian
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Blocker cleared — generated artifacts regenerated

Pushed bdd0b24. The required Verify generated artifacts step now passes.

Root cause of the drift, and a trap worth recording

The committed bundles were last rebuilt at d141994 and did not contain this PR's own new symbols. That was not merely a red light: package.json maps bin.knowledge-mcpbin/knowledge-mcp.js, so the MCP half of the fix did not exist in the artifact that actually executes — a half-configured client reached through the MCP would still have silently fallen back to the on-box store.

The first rebuild attempt reproduced the symlinked-node_modules trap flagged in review, and it is worse than "local-only noise". This worktree's node_modules was a symlink to the parent checkout, so bun embedded escaped relative paths into the module-header comments:

-// node_modules/@hasna/contracts/dist/auth/index.js
+// ../../../../../workspace/hasna/opensource/open-knowledge/node_modules/@hasna/contracts/dist/auth/index.js

Committing that would have leaked a local filesystem path into the published bundle and left CI red, while spuriously touching four extra files. Fixed by replacing the symlink with a real bun install --frozen-lockfile and rebuilding at bun 1.3.14, matching the bun-version pinned in .github/workflows/ci.yml.

Positive control — the symbols are actually in the bundles

Grepped after the clean rebuild, not inferred from a zero exit code:

artifact HalfConfiguredKnowledgeClientError assertKnowledgeModeSelected
bin/knowledge.js 1 0 — name-mangled by --minify
bin/knowledge-mcp.js 3 2
bin/knowledge-serve.js 0 0
dist/index.js 0 0

The zeros are correct rather than a build miss: only src/cli.ts and src/mcp.js reach the new symbols (src/knowledge-db.ts imports just the unchanged KNOWLEDGE_MODE_ENV_KEYS). In the minified CLI bundle the class survives as this.name="HalfConfiguredKnowledgeClientError".

The resulting diff is exactly three files — bin/knowledge-mcp.js (+21), bin/knowledge.js (binary, minified), dist/knowledge-mode.d.ts (+31, tsc typing emit). bin/knowledge-serve.js and dist/*.js came out byte-identical to what was already committed, which independently confirms the earlier churn on those was purely the symlink artifact.

Local gate at the pinned bun with real node_modules:

verify-generated-artifacts: 6 generated bundles rebuild byte-identically and carry no stale generated code.
EXIT=0

Scope

Only the regenerated artifacts. The five P2/P3 review findings are untouched and remain filed as separate follow-ups.

Agent: vespasian

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

CI green on bdd0b24

Run 30554393074conclusion: success, all 7 jobs pass:

success  test (macos-latest, node)          <- the blocker, was failing at "Verify generated artifacts"
success  test (macos-latest, bun)
success  test (ubuntu-latest, node)
success  test (ubuntu-latest, bun)
success  test-matrix (macos-latest, bun)
success  test-matrix (ubuntu-latest, bun)
success  test-matrix (windows-latest, bun)  <- 3m03s, passed first try

On the Windows leg: the earlier sync peer-workspace works without machines adapter calls failure did not recur. The push produced a fresh Windows run and it passed on its own, so no separate re-run was issued — the flake diagnosis holds.

[code]smith reports skipping; it is not a required check.

Not merging — disposition is vespasian's.

Agent: vespasian

@andrei-hasna
andrei-hasna merged commit 4c4d6c1 into main Jul 30, 2026
8 checks passed
@andrei-hasna
andrei-hasna deleted the fix/b71ec0eb-loud-half-configured-client branch July 30, 2026 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant